Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jest diff numbers and booleans #7605

Merged
merged 5 commits into from
Jan 20, 2019

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Jan 10, 2019

Summary

Currently, jest-diff returns null when diffing numbers or booleans (or NO_DIFF_MESSAGE if they are equal).
This is because the matchers that use jest-diff already print something like

Expected: 2
Received: 1

if they differ, so the jest-diff output would be redundant.

This behavior is unintuitive if you are using jest-diff for something other than Jest's test failure output.
I stumbled upon this trying to build a diff-like CLI for jest-diff.
It was also quite inexplicable to me when I first read the jest-diff code ("why does it just not do anything if a and b are numbers?"), so I think this piece of code needed clarification in general.

This PR changes jest-diff to behave like normal diffing by default, with an omitTrivial option that the matchers use to tell jest-diff not to generate a diff between numbers or booleans.

Test plan

Added tests for numbers and booleans to diff.test.
e2e tests unchanged, confirming that this does not change the overall behavior of Jest.
One exception: Along the way, I also fixed the following contradictory output :D

Expected: 0
Received: -0

Difference:

Compared values have no visual difference.

@SimenB
Copy link
Member

SimenB commented Jan 10, 2019

Thanks again @jeysal! @pedrottimark how does this fit into #6961 and #7557?

@jeysal
Copy link
Contributor Author

jeysal commented Jan 11, 2019

Yeah I saw those two open PRs too - might have small merge conflicts but it's semantically different concerns so I think we should be fine.

@pedrottimark
Copy link
Contributor

pedrottimark commented Jan 11, 2019

Definite yes to the change from === operator to Object.is call.

The exceptions of null and the two message strings as return values seem like too close coupling between jest-diff and expect packages. I like the idea of options for a CLI utility as a way to think about what to do instead. To help me comment more effectively:

@jeysal can you summarize the pros and cons of adding a new option to keep the current behavior (therefore a breaking change) versus to give the improvement (therefore opt-in and not a breaking change). According to npm there are 88 dependents.

EDIT: To clarify a possible misunderstanding, I don’t mean that users of CLI tool shouldn’t need to specify an option to get desired behavior.

packages/jest-diff/src/index.js Outdated Show resolved Hide resolved
@jeysal
Copy link
Contributor Author

jeysal commented Jan 12, 2019

@jeysal can you summarize the pros and cons of adding a new option to keep the current behavior (therefore a breaking change) versus to give the improvement (therefore opt-in and not a breaking change). According to npm there are 88 dependents.

Con - the breaking change 😄
Pro

  • It's super confusing that it returns null for numbers, especially given that there is no documentation.
    • In consequence, I don't think many dependents rely on this behavior. Most will probably be surprised to hear that jest-diff does nothing useful for numbers and booleans.
  • As mentioned in the OP, I also think that it "cleans up" the Jest codebase itself (despite adding more code). Just the existence of {omitTrivial: true} in the calls and if (omitTrivial) in the impl leads you to look up what the option does in DiffOptions instead of wondering why these strange exceptions exist.

@jeysal
Copy link
Contributor Author

jeysal commented Jan 12, 2019

On the too close coupling:
I agree that this is currently not ideal. Giving jest-diff a more reasonable default behavior like this PR does would reduce the coupling a little IMO.
Alternatively:
Maybe we should just have expect (and other jest-diff consumers) not call diff at all if they're dealing with something like number comparisons?

@thymikee
Copy link
Collaborator

Maybe we should just have expect (and other jest-diff consumers) not call diff at all if they're dealing with something like number comparisons?

I think in the long run we want jest-diff to be able to produce a diff with highlights, which would be nice when comparing bigger numbers/strings :)

@jeysal
Copy link
Contributor Author

jeysal commented Jan 12, 2019

I think in the long run we want jest-diff to be able to produce a diff with highlights, which would be nice when comparing bigger numbers/strings :)

Yeah that would be cool - then we could remove the special handling altogether. Do you think that as it is the number/boolean special handling should be in jest-diff behind omitTrivial or in expect etc. where the "Expected x to be y" is printed and thus the redundant output created?

@jeysal jeysal force-pushed the jest-diff-numbers-and-booleans branch from 035b03c to 23dc84f Compare January 12, 2019 13:09
@thymikee
Copy link
Collaborator

I think I'm leaning towards the no-omitTrivial scenario where expect decides about how to show the difference. omitTrivial seems hacky and it's only to facilitate current behavior, which is not ideal. I'm cool however with using this as a temporary solution and adjust matchers later.

With inline highlights we could have the output from the matcher like this:

expected(received).toBe(expected)

Expected: 1234567894123 // green with highlights
Received: 1230061194132 // red   with highlights

since it would know we only have 2 single-line values to compare which would be so awesome.

Anyway, @pedrottimark has it probably all figured out couple of months ahead, so he's the best person to answer

@jeysal
Copy link
Contributor Author

jeysal commented Jan 12, 2019

Yeah I think that would be better as well. If we go this route, probably best to do it now though, otherwise we have a breaking change again when we remove omitTrivial. Shouldn't be that hard to implement, unless @pedrottimark sees a problem with that approach that I missed 😄

@codecov-io
Copy link

Codecov Report

Merging #7605 into master will increase coverage by 0.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7605      +/-   ##
==========================================
+ Coverage   68.26%   68.27%   +0.01%     
==========================================
  Files         249      249              
  Lines        9623     9627       +4     
  Branches        6        6              
==========================================
+ Hits         6569     6573       +4     
  Misses       3052     3052              
  Partials        2        2
Impacted Files Coverage Δ
packages/jest-circus/src/formatNodeAssertErrors.js 18.18% <ø> (ø) ⬆️
packages/jest-snapshot/src/index.js 52.63% <ø> (ø) ⬆️
packages/jest-diff/src/diffStrings.js 98.52% <ø> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/Spec.js 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
packages/expect/src/matchers.js 99.43% <100%> (ø) ⬆️
packages/expect/src/spyMatchers.js 97.44% <100%> (ø) ⬆️
packages/jest-diff/src/index.js 84.09% <100%> (+1.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f425bd4...23dc84f. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Jan 20, 2019

+1 to not add a omitTrivial. Would be awesome to land in time for Jest 24, which will (barring anything unforeseen happening) happen this week.

@jeysal
Copy link
Contributor Author

jeysal commented Jan 20, 2019

Okay, I'll try to build the required expect behavior in time for Jest 24.

@SimenB
Copy link
Member

SimenB commented Jan 20, 2019

We can regress a tiny bit on the output from expect and solve that in a patch release in a week, if it turns out to be time consuming. But the api change in jest-diff needs to land before that 🙂

(preferable would be for both to land at the same time, of course)

@pedrottimark
Copy link
Contributor

Thank you for patience while this simmered in my mental slow cooker.

I will share some thoughts this afternoon.

For long view, see #7603 (comment)

@jeysal jeysal force-pushed the jest-diff-numbers-and-booleans branch from 23dc84f to 367343a Compare January 20, 2019 17:00
@jeysal
Copy link
Contributor Author

jeysal commented Jan 20, 2019

I've removed all things omitTrivial and instead built a shouldPrintDiff function in jest-matcher-utils that tells expect not to print a diff when comparing two numbers or two booleans (and that could be extended if we want to omit the diff on other pairs of values in the future).
Would appreciate if everyone involved takes a look at this new implementation :)

@jeysal
Copy link
Contributor Author

jeysal commented Jan 20, 2019

Oh right, node assert...

@SimenB
Copy link
Member

SimenB commented Jan 20, 2019

Node assert code should probably live in jest-matcher-utils instead of being duplicated across jest-jasmine and jest-circus. Maybe for another PR, though 🙂

@jeysal
Copy link
Contributor Author

jeysal commented Jan 20, 2019

Given that jest-jasmine2 assertionErrorMessage.js and jest-circus formatNodeAssertErrors.js will also need

shouldPrintDiff(a, b) ? jestDiff(a, b, options) : null

, maybe extracting it into jest-matcher-utils is not so bad after all, even though it would add the jest-matcher-utils => jest-diff dependency?
Anyone opposed to adding the dependency? jest-matcher-utils already has
https://github.com/facebook/jest/blob/198282580f3aa9cf87465e208dc514f3d7a4546e/packages/jest-matcher-utils/package.json#L15-L19

@SimenB
Copy link
Member

SimenB commented Jan 20, 2019

I'm fine with that as a dep, seems natural that a matcher utility can also do diffing

@jeysal jeysal force-pushed the jest-diff-numbers-and-booleans branch from bb9a782 to 6f7d209 Compare January 20, 2019 17:47
@jeysal
Copy link
Contributor Author

jeysal commented Jan 20, 2019

Done. This also removed two "Compared values have no visual difference."s in failures.test.js, but that looks alright to me

@jeysal
Copy link
Contributor Author

jeysal commented Jan 20, 2019

Also using the diff from jest-matcher-utils in spyMatchers.
The only remaining direct consumer of jest-diff is now jest-snapshot.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you!

@SimenB SimenB merged commit b450d78 into jestjs:master Jan 20, 2019
@pedrottimark
Copy link
Contributor

@jeysal Brilliant solution to make the change available to other dependents!

Look forward to collaborating on future improvements to reports in Jest, if you are interested :)

@jeysal jeysal deleted the jest-diff-numbers-and-booleans branch January 20, 2019 19:55
@jeysal jeysal mentioned this pull request Feb 2, 2019
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants